Skip to content

Automatic lookup of DOI at citation relations #13539

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

ankamde
Copy link
Contributor

@ankamde ankamde commented Jul 14, 2025

Closes #13234

In the "Citation relations", if there is no DOI, offer a "link" to determine the DOI.

Steps to test

  1. In the "{} biblatex source" add an article

@Article{,
author = {Oliver Kopp and Carl Christian Snethlage and Christoph Schwentker},
journal = {TUGboat},
title = {JabRef: BibTeX-based literature management software},
year = {2023},
issn = {0896-3207},
number = {3},
pages = {441--447},
volume = {44},
issue = {138},
ranking = {rank4},
}

image
  1. "Citation relations" tab shows modified message with "Look up a DOI and try again." being a link on both panels.
image
  1. Click on one of the "Look up a DOI and try again." - DOI look up will start.
image
  1. After successful look up both panels are populated with the data (if exists).
image
  1. "General" contains looked up DOI.
image

Negative tests

  1. When DOI cannot be found, display a message to the user.
image
  1. In case on an error - show message on the panels.
image

Mandatory checks

  • I own the copyright of the code submitted and I license it under the MIT license
  • Change in CHANGELOG.md described in a way that is understandable for the average user (if change is visible to the user)
  • [/] Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (if change is visible to the user)
  • Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
  • Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

Button refreshButton,
CitationFetcher.SearchType searchType,
Button importButton,
ProgressIndicator progress) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UI component (ProgressIndicator) should not be directly included in a data structure. This violates separation of concerns and should be managed in a dedicated UI controller.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is simple parameter object - instead passing 7 parameters, use dedicated object.

ObservableList<CitationRelationItem> observableList = FXCollections.observableArrayList();
citationComponents.listView().setItems(observableList);

// TODO: It should not be possible to cancel a search task that is already running for same tab
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment does not add new information and is just stating what should be done. TODO comments should be tracked in issue tracking system instead of code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All TODO: comments were in this class before refactoring - I'm happy to add all 3 to issue tracking system but I think I don't have sufficient priviledges.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you move things around? Makes things hard to review.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I grouped methods that collaborate. It was slightly difficult for me to navigate this class in the previous form.

@@ -2791,7 +2791,8 @@ Miscellaneous=Diversos
File-related=Arquivo relacionado

Add\ selected\ entry(s)\ to\ library=Adicionar as referências selecionadas à biblioteca
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have to go through crowdin web site to update translations....

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, let me have a look.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the PR is merged, the new translations will be synced with crowdin and you can translat them there https://crowdin.com/project/jabref

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@koppor @Siedlerchr Could you raise an issue to address the new translations with Crowdin and assign it to me, please?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need that for a university course? Typically Crowdin just works.

hideNodes(citationComponents.abortButton(), citationComponents.progress(), citationComponents.importButton());
showNodes(citationComponents.refreshButton());
task.cancel();
dialogService.notify(Localization.lang("Search aborted!"));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use of exclamation mark in UI text notification violates the guideline for avoiding exclamation marks at sentence endings in UI messages.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This message was here before I refactored this class. Looks like a valid notification to me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could change the localization string to Search aborted..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@ankamde ankamde marked this pull request as ready for review July 16, 2025 14:22
@ankamde
Copy link
Contributor Author

ankamde commented Jul 16, 2025

@koppor @Siedlerchr This PR is ready for a review. Please have a look.

Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comments

I need to start RefactoringMiner to be able really review the moved around methods.


HBox hBox = new HBox();
Label label = new Label(Localization.lang("The selected entry doesn't have a DOI linked to it."));
Hyperlink link = new Hyperlink(Localization.lang("Look Up a DOI and try again."));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use sentence case in JabRef. Change Up to up. -- Also at the lines below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

ObservableList<CitationRelationItem> observableList = FXCollections.observableArrayList();
citationComponents.listView().setItems(observableList);

// TODO: It should not be possible to cancel a search task that is already running for same tab
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you move things around? Makes things hard to review.

@ankamde ankamde changed the title Issue 13234 automatic lookup of doi at citation relations Issue 13234 automatic lookup of DOI at citation relations Jul 16, 2025
@ankamde
Copy link
Contributor Author

ankamde commented Jul 16, 2025

@koppor I've addressed all of the comments. Please have a look.

@ankamde ankamde requested review from koppor and Siedlerchr July 17, 2025 07:24
Copy link

trag-bot bot commented Jul 18, 2025

@trag-bot didn't find any issues in the code! ✅✨

@calixtus calixtus changed the title Issue 13234 automatic lookup of DOI at citation relations Automatic lookup of DOI at citation relations Jul 19, 2025
@koppor koppor enabled auto-merge July 20, 2025 22:43
Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked the diff in Refactoring Miner - and also tried it out.

Looks good.

@koppor koppor added this pull request to the merge queue Jul 20, 2025
Merged via the queue into JabRef:main with commit e1d0d66 Jul 20, 2025
44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automatic lookup of DOI at citation relations
3 participants